Skip to content

Resolve relative local collection sources - #3124

Closed
OskarEichler wants to merge 1 commit into
ruby:masterfrom
OskarEichler:codex/relative-local-source
Closed

Resolve relative local collection sources#3124
OskarEichler wants to merge 1 commit into
ruby:masterfrom
OskarEichler:codex/relative-local-source

Conversation

@OskarEichler

Copy link
Copy Markdown
Contributor

Summary: resolve local collection source paths relative to the configuration directory and expand the result, instead of anchoring them to the process working directory.

Verification: focused baseline/fixed reproduction, combined RBS 4.2.0 consumer models, and RuboCop (738 files, zero offenses). No tests are added in this PR.

Compatibility: no public API removal or dependency/version change.

@pocke

pocke commented Sep 7, 2026

Copy link
Copy Markdown
Member

Thank you for the patch. I am closing this in favour of #3146, which takes the comment deletion on its own.

You were right to question the TODO: it has been stale since 2023. I dug through the history of #854 and wrote it up in #3146. The short version is that the comment arrived in a 2022-02-11 force-push, when the constructor really did take the path as written, and it was answered on 2023-02-08 by the commit that added the base_directory: keyword. Only the code was updated. Because #854 was squashed into a single commit, the leftover is invisible from git log -L on the file, which is presumably why it survived.

The reason I am splitting it is the other half of this PR. (base_directory / path).expand_path is a real change with its own behaviour, and it deserves to be judged separately:

  • It only does anything when base_directory is relative. When it is absolute, Pathname#+ has already collapsed any .. lexically, so expand_path is a no-op. I checked this with a path: ../shared_sig source: the symlink target is byte-identical with and without your change, so existing installations see no churn.
  • The rbs CLI never produces a relative base_directory. config_path is either an absolute path from Config.find_config_path or the expanded argument of --collection. I could only reproduce the broken symlink by building a Lockfile with a relative lockfile_path directly, which is what a caller like Steep can do.
  • When I did reproduce it, the bug was clear: full_path stayed sig, so File.symlink wrote sig/ast/2.4 as the target, which the kernel then resolved relative to the symlink's own directory rather than the working directory, leaving a dangling link.

So the change is correct and worth having. What it needs is a test that builds the relative case and shows the link resolving, which would also pin down that the CLI is not affected. If you would like to open that as its own PR, I would be glad to review it.

@pocke pocke closed this Sep 7, 2026
hayat01sh1da pushed a commit to hayat01sh1da/rbs that referenced this pull request Sep 7, 2026
`Sources::Local#initialize` carries a TODO asking to resolve a relative
source path from the directory of `rbs_collection.yaml`. That was done
three and a half years ago and the comment was left behind.

The TODO arrived in 61dd0e4, a force-push of ruby#854 dated 2022-02-11,
where the constructor was:

    def initialize(source_entry)
      # TODO: resolve relative path from dir of rbs_collection.yaml
      @path = Pathname(source_entry['path'])
    end

There the path really was taken as written and resolved against the
working directory, so the TODO described the code accurately.

86f4ec5, pushed to the same PR on 2023-02-08, added the
`base_directory:` keyword to `Sources.from_config_entry` and changed the
body to `base_directory / path`. `Config#sources` passes
`@config_path.dirname` and `Lockfile.from_lockfile` passes
`lockfile_path.dirname`, so from that point a relative path has been
resolved from the directory holding the configuration -- exactly what
the TODO asked for. Only the code was updated; the comment stayed.

ruby#854 ran from 2021-12-23 to 2023-02-17 and its commits were squashed
into 718997e, so the TODO and the change that answered it landed in one
commit and the leftover was invisible in the history.

Close ruby#3124

ruby#854

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HSq3kKMB8ULhZrnL5urD7y
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants